Wouldn't it be better to test the result of view.instance_exec(&menu_item.condition) in a couple of different scenarios? With your current test, one could change all the || to && and still have the test pass.

Of course, this is a trade-off between simplicity and completeness. Although I think the change to && would make the test fail (can? would be called just once), I see the point, I'm just not sure how much more detail we want to put in there, given the complete matrix in all its combinations is probably too much.

I think the real fix would be to redesign the admin menu items to have sub-items wrapped in objects and check if any of those is #visible?, but this is clearly beyond the scope of this PR.

That'd be fine if I opened an issue describing the underlying problem?

Although I think the change to && would make the test fail (can? would be called just once)

Right, my bad. 🤦

I think the real fix would be to redesign the admin menu items to have sub-items wrapped in objects and check if any of those is #visible?, but this is clearly beyond the scope of this PR.

Agreed that it would be better. Feel free to open an issue.

I see the point, I'm just not sure how much more detail we want to put in there, given the complete matrix in all its combinations is probably too much.

I feel like we could do a bit better than the current implementation without writing too much additional code.

A good compromise could be just two tests such as the following:

it 'is shown if any of its submenus are present' do
  view = double("view")
  allow(view).to receive(:can?).and_return(true, false)

  result = view.instance_exec(&menu_item.condition)

  expect(result).to eq(true)
end

it 'is not shown if none of its submenus are present' do
  view = double("view", can?: false)

  result = view.instance_exec(&menu_item.condition)

  expect(result).to eq(false)
end

The main benefits over the existing tests, in my opinion, are:

1. You're testing the underlying logic more directly. Rather than setting an expectation on how many times can? is called, you're setting it on whether the condition works properly.
2. The tests are more expressive. Reading your test, it's unclear how the underlying implementation works. With this version, it's a bit clearer what's going on in the implementation.
3. The tests follow a standard setup/exercise/verify pattern and are easier for a human to parse. The origin.al test mixed setup and expectations by using expect(view).to receive(...).

Let me know your thoughts!

Thanks for the suggestion, it was useful!
The issue is here #3785